-
-
Notifications
You must be signed in to change notification settings - Fork 65
ITP_LONDON | PRISCILLA_EMEBO | Module-Data-Flows | Book library #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ITP_LONDON | PRISCILLA_EMEBO | Module-Data-Flows | Book library #183
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some improvements you can still make to the JS code.
Also, when I checked the code in index.html
at https://validator.w3.org/, it indicated some errors.
debugging/book-library/script.js
Outdated
author.value == null || | ||
author.value == "" || | ||
pages.value == null || | ||
pages.value == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of input validation,
- Are all input properly checked?
- Can
.value
benull
? (Do we need to checksomeInputElement.value == null
?) - What if a user enters only space characters in the "title" input field?
- What if a users enters
-1
or3.1416
in the "pages" input field?
debugging/book-library/script.js
Outdated
for (let n = rowsNumber - 1; n > 0; n-- ){ | ||
table.deleteRow(n); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>
) in the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 71: With the way the book's title is assigned to an HTML element, a book with a title containing special character sequence such as <i>
can possibly ruin the display.
Lines 77, 95:
- Is the value assigned to these
id
attribute unique? - Is there a need to assign an id attribute to
delButton
? - Is there a need to assign an id attribute to
changeBut
(at line 72)?
Lines 76, 94:
- Can you think of a more consistent way to name the variables representing the two buttons,
changeBut
anddelButton
?
Learners, PR Template
Self checklist
Changelist
This PR implements a simple Book Library web app using HTML, CSS, and JavaScript. Users can add books with a title, author, page count, and read status. Each book is displayed in a table, with options to toggle its read status or delete it
Questions
Ask any questions you have for your reviewer.